Skip to content

fix: validate chunk order and identity when deserializing chunked transactions - #2736

Open
danielmarv wants to merge 5 commits into
hiero-ledger:mainfrom
danielmarv:fix/validate-chunked-transaction-list
Open

danielmarv wants to merge 5 commits into
hiero-ledger:mainfrom
danielmarv:fix/validate-chunked-transaction-list

Conversation

@danielmarv

Copy link
Copy Markdown
Member

Description:

Transaction.from_bytes() accepted a TransactionList for a chunked transaction (FileAppendTransaction, TopicMessageSubmitTransaction) whose chunk groups were reordered or belonged to different chunked transactions. The restored transaction then re-serialized and executed the chunks in input order: file contents were appended out of order, and topic message chunks were submitted out of sequence.

  • Validate the chunk groups of chunked transaction types:
    • all chunk transaction IDs share the same payer account;
    • transactionValidStart strictly increases from one chunk group to the next;
    • consensusSubmitMessage only: chunkInfo.number is the chunk's 1-based position, chunkInfo.total equals the number of chunk groups, and chunkInfo.initialTransactionID equals the first chunk's transaction ID.
  • from_bytes() only falls back to the single Transaction format when the bytes do not decode to a non-empty TransactionList. Validation errors now reach the caller as a ValueError with the real cause, instead of being replaced by an unrelated parse error.
  • Split the private _process_transaction_list_bytes() into _parse_transaction_list() and _process_transaction_list(). No public API changes.

Related issue(s):

Fixes #2710

Notes for reviewer:

  • Root cause: transaction_ids is collected in first-seen input order, and _validate_transaction_pairs() builds its expected layout from it, so a reordered list always matched itself. The new checks read each chunk group's own transactionID and chunkInfo instead.
  • Reject, don't sort: malformed lists are rejected, not silently reordered.
  • Strictly increasing, not an exact +1ns step: Java and the JS TopicMessageSubmitTransaction space chunk valid starts 1ns apart, but the JS FileAppendTransaction uses a configurable chunkInterval (default 10ns). Neither carries nanos overflow into seconds, so valid starts are compared as (seconds, nanos).
  • Single-chunk topic messages: Python and Java omit chunkInfo for a single chunk, while JS sets total=1, number=1. Both are accepted. A lone chunk of a larger message (e.g. total=2) is now rejected as incomplete.
  • Bodies without node account IDs: the chunk checks also run on this path, which previously skipped chunk validation entirely.
  • Known limit: FileAppendTransaction carries no chunk count, so a list with its last chunk(s) removed cannot be detected. Topic messages are covered by chunkInfo.total.
  • Updated existing tests: four tests built chunk sequences that are now invalid; they now use sequential chunk IDs:
    • test_validate_transaction_bodies_chunk_transaction and test_validate_transaction_bodies_chunk_transaction_mutiple_node_ids used different payers (0.0.2 and 0.0.3);
    • test_validate_transaction_bodies_for_chunk_file_append_tx and test_validate_transaction_bodies_for_chunk_message_submit_tx built chunk IDs with TransactionId.generate(), which subtracts a random 5–8s, so the second chunk was often earlier than the first. The topic test also gave both chunks number=1 and unrelated initialTransactionIDs.
  • Fixed the transaction_ids annotation in _process_transaction_list() (list[Transaction] → list[TransactionId]).
  • Tests & Linters:
    • New tests/unit/chunked_transaction_from_bytes_test.py (66 cases) tests through Transaction.from_bytes() for both types, with one and two nodes:
      • round-trips of 1–3 chunks, signed and unsigned, give identical bytes, and 10ns chunk spacing is accepted;
      • swapped chunk groups, different payers, non-increasing valid starts, wrong chunkInfo.number/total, mismatched initialTransactionID, missing chunkInfo and an incomplete topic message are rejected with a specific message. All 34 rejection cases fail against main.
    • New tests in transaction_freeze_and_bytes_test.py for the single Transaction fallback (previously untested) and for a TransactionList validation error reaching the caller.
    • uv run pytest tests/unit: 3404 passed.
    • All pre-commit hooks (gitleaks, end-of-file-fixer, trailing-whitespace, ruff-check, ruff-format, bandit) pass.
    • Integration tests have not been run locally.

Checklist

  • Documented (Code comments, README, etc.)
  • Tested (unit, integration, etc.)

…nsactions

Transaction.from_bytes() now rejects a TransactionList for FileAppendTransaction or TopicMessageSubmitTransaction unless its chunk groups share one payer, have strictly increasing valid starts and, for topic messages, carry chunk info matching the chunk sequence. The checks are derived from each chunk group's body rather than from the transaction IDs collected in first-seen order, which always matched the input order and let reordered chunk groups pass.

from_bytes() now only falls back to parsing a single Transaction when the bytes do not decode to a non-empty TransactionList, so validation errors reach the caller instead of being replaced by an unrelated parse error.

Signed-off-by: Ntege Daniel <danientege785@gmail.com>
@danielmarv
danielmarv requested review from a team as code owners October 18, 2026 15:28
@github-actions github-actions Bot added approved Issue has been approved by team member skill: intermediate requires some knowledge of the codebase with some defined steps to implement or examples labels Oct 18, 2026
@codecov

codecov Bot commented Oct 18, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 96.07843% with 2 lines in your changes missing coverage. Please review.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2736      +/-   ##
==========================================
+ Coverage   95.76%   95.77%   +0.01%     
==========================================
  Files         172      172              
  Lines       11274    11313      +39     
==========================================
+ Hits        10796    10835      +39     
  Misses        478      478              
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@github-actions github-actions Bot added open to community review PR is open for community review and feedback queue:junior-committer PR awaiting initial quality review labels Oct 18, 2026
@coderabbitai

coderabbitai Bot commented Oct 18, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: hiero-ledger/hiero-sdk-python/.coderabbit.yaml
  • Review profile: ASSERTIVE
  • Plan: Advanced
  • Run ID: ac1238a6-23b0-458b-8b16-ced592949797
📥 Commits

Reviewing files that changed from the base of the PR and between b64d706 and 24b5c3a.

📒 Files selected for processing: 2
  • src/hiero_sdk_python/transaction/transaction.py
  • tests/unit/transaction_freeze_and_bytes_test.py

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

Walkthrough

Transaction.from_bytes now preserves validation errors from non-empty TransactionList data. Chunk validation checks shared payer IDs, increasing valid-start timestamps, ordering, and topic-message metadata. Tests cover valid restoration and invalid chunk sequences.

Changes

Transaction restoration

Layer / File(s) Summary
TransactionList deserialization
src/hiero_sdk_python/transaction/transaction.py, tests/unit/transaction_freeze_and_bytes_test.py
Transaction.from_bytes falls back to single-transaction parsing only when list decoding fails or produces an empty list. Tests cover concrete-type restoration and mixed-type validation errors.
Chunk identity and sequence validation
src/hiero_sdk_python/transaction/transaction.py, tests/unit/chunked_transaction_from_bytes_test.py, tests/unit/file_append_transaction_test.py, tests/unit/topic_message_submit_transaction_test.py, tests/unit/transaction_test.py
Chunk validation requires a shared payer and strictly increasing valid-start timestamps. Topic-message chunks require matching numbers, totals, and initial transaction IDs. Validation also runs without node IDs. Tests cover round trips, accepted gaps, and rejected sequences.

Priority: ➖ Normal

Estimated code review effort: 4 (Complex) | ~45 minutes

Severity of issue fixed: Medium

Sequence Diagram(s)

sequenceDiagram
  participant Caller
  participant Transaction.from_bytes
  participant TransactionList decoder
  participant Chunk validation
  Caller->>Transaction.from_bytes: Provide serialized bytes
  Transaction.from_bytes->>TransactionList decoder: Decode TransactionList
  TransactionList decoder-->>Transaction.from_bytes: Return decoded list or decode failure
  Transaction.from_bytes->>Chunk validation: Validate chunk identity and metadata
  Chunk validation-->>Transaction.from_bytes: Return validation result
  Transaction.from_bytes-->>Caller: Return restored transaction or validation error
Loading
Merge Risk: ⚪ Minimal · up to 24b5c

No actionable merge-blocking issue is identified; proceed with normal checks.

Security architecture risk: ⚪ Minimal · up to b64d7

The change strengthens transaction validation without adding execution authority or broadening access. The review found no material security risk introduced or worsened by the PR. Existing limits on proving file-append completeness remain unchanged.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The relevant exposure is caller-supplied serialized transactions later used for file appends or topic submissions. The change narrows accepted chunk arrangements and does not add credentials, network destinations, or operator authority. Execution still conditions operator signing on payer identity and submits through the client network.

Trust Boundaries and Controls

  • observed — For lists reaching chunk validation, the controls require a common payer and strictly increasing valid starts. Topic chunks additionally require matching sequence numbers, total count, and initial transaction ID. Existing node-pair uniqueness and per-node body consistency checks remain ahead of chunk validation.

Resilience and Maintainability Implications

  • inferred — A parse or validation failure does not publish the partially accumulated restored object. Signatures and body maps are populated locally before validation, but the caller receives the object only after validation and finalization. Existing downstream retry and network-level atomicity limitations are not changed by this restoration refactor.
Pre-merge checks: Passed 4 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Docstring Coverage Warning Docstring coverage is 79.55% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 44 functions across 6 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
Passed Passed checks: 4 passed
Check name Status Explanation
Title check Passed The title clearly and concisely describes the main change: validating chunk order and identity during deserialization.
Description check Passed The description directly explains the deserialization bug, validation rules, error propagation, tests, and scope of the changes.
Linked Issues check Passed Issue #2710 requirements are met. Transaction.from_bytes() now falls back only when decoding does not produce a non-empty TransactionList, so validation ValueErrors reach the caller. Chunk valid…
Out of Scope Changes check Passed The changes stay within issue #2710. Production changes add the requested chunk validation, fallback error propagation, and private helper split. Test changes add required coverage and correct fixture…
Full details: Linked Issues check

Explanation

Issue #2710 requirements are met. Transaction.from_bytes() now falls back only when decoding does not produce a non-empty TransactionList, so validation ValueErrors reach the caller. Chunk validation checks shared payer accounts and strictly increasing (seconds, nanos) valid starts. Topic message validation checks 1-based numbers, total count, and the first transaction ID, while accepting the documented single-chunk forms. The implementation rejects malformed input and does not reorder it. Tests exercise both chunked transaction types through Transaction.from_bytes(), signed and unsigned round trips, one and multiple nodes, single chunks, missing node IDs, and the required rejection cases. Private helpers are used and no public API change is shown.

Full details: Out of Scope Changes check

Explanation

The changes stay within issue #2710. Production changes add the requested chunk validation, fallback error propagation, and private helper split. Test changes add required coverage and correct fixtures that were invalid under the new rules. No unrelated production behavior or public API change is shown.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@aceppaluni aceppaluni left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we add a regression test using a real serialized single Transaction that is also successfully decoded as a non-empty TransactionList?

Since protobuf parsing doesn't necessarily distinguish these two wire formats, checking only transaction_list.transaction_list may incorrectly classify the input and skip the single-transaction fallback.

What do you think?

@danielmarv

Copy link
Copy Markdown
Member Author

Thanks @aceppaluni
TransactionList only defines field 1, so a single Transaction decodes as a non-empty list only if it sets its own field 1, the deprecated body, which _process_base_transaction() can't restore anyway. The formats we do restore, signedTransactionBytes (field 5) and bodyBytes + sigMap (fields 4/3), always decode to an empty list and take the fallback. This matches Java (if (!list.getTransactionListList().isEmpty())) and JS (if (list.length === 0)).

test_from_bytes_accepts_single_transaction_bytes already covers a real serialized signedTransactionBytes transaction. I've added a regression test for the legacy bodyBytes + sigMap format, plus one showing a deprecated body transaction is rejected with a ValueError.

@github-actions

Copy link
Copy Markdown

Hello, this is the OfficeHourBot.

This is a reminder that the Hiero Python SDK Office Hours will begin in approximately 3 hours and 56 minutes (14:00 UTC).

This session provides an opportunity to ask questions regarding this Pull Request.

Details:

Disclaimer: This is an automated reminder. Please verify the schedule here for any changes.

From,
The Python SDK Team

@danielmarv
danielmarv requested a review from aceppaluni October 21, 2026 10:23
@manishdait manishdait added this to the v0.3.1 milestone Oct 21, 2026

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Issue has been approved by team member open to community review PR is open for community review and feedback queue:junior-committer PR awaiting initial quality review skill: intermediate requires some knowledge of the codebase with some defined steps to implement or examples

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Validate chunk order and identity when deserializing chunked transactions from a TransactionList

4 participants